[No QA] Use full Git history to populate StagingDeployCash #3173

roryabraham · 2021-05-27T01:37:13Z

Details

The main motivation behind this pull request is to use the full Git / GitHub history to populate the StagingDeployCash, rather than relying on one-off changes made in actions. This makes the whole process more robust: any time the StagingDeployCash is updated, it should have all the correct pull requests and deploy blockers. That means if there is a GH outage, then when the service is restored the StagingDeployCash should self-resolve to its correct state.

Here's a more detailed list of the changes made in this PR (in broad strokes)

Changes GitUtils.getPullRequestsMergedBetween to be synchronous instead of async. Before it was unnecessarily async, and an arbitrary source of extra complexity.
Removes createNewStagingDeployCash and updateStagingDeployCash from the GithubUtils class. This is now handled entirely in the createOrUpdateStagingDeploy GitHub Action (located at .github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js). Accordingly, I stripped some tests from GitHubUtilsTest and created a new automated test for the createOrUpdateStagingDeploy GH Action.
Removes the NEW_PULL_REQUESTS and NEW_DEPLOY_BLOCKERS inputs from the createOrUpdateStagingDeploy action. Instead, the action finds pull requests by looking at the tag on the previous StagingDeployCash, and parsing the git history between that tag and the latest tag to determine which PRs were merged since the last prod deploy. It finds deploy blockers by just querying the GH api for any open issues with the DeployBlockerCash label.
Updates the YML workflows to remove those unused inputs from usages of the createOrUpdateStagingDeploy action.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/164517

Tests

Merge this pull request.
Verify that a staging deploy happens and that the StagingDeployCash is updated with this pull request and the new BUILD version.
Check off this pull request on the StagingDeployCash
Lock the StagingDeployCash, then immediately after merge a pull request.
Verify that the StagingDeployCash is updated to include that pull request.
Merge another pull request.
Verify that the the pull request gets the "deploy deferred" comment.
Verify that the pull request is not deployed to staging and is not added to the StagingDeployCash
Create a new issue labelled DeployBlockerCash
Verify that the StagingDeployCash is updated to include the new deploy blocker
Label this pull request with DeployBlockerCash
Verify that the StagingDeployCash is updated to list this pull request as a deploy blocker.
Unlock and relock the StagingDeployCash.
Verify that the StagingDeployCash is update to include the deferred pull request.
Verify that the items on the StagingDeployCash that were checked off remain checked off.
Merge another pull request. Verify that it gets the "deploy deferred" comment and is not deployed to staging / added to the StagingDeployCash.
Close the StagingDeployCash to trigger a production deploy. Verify that the prod deploy begins.
Verify that a new StagingDeployCash is created, including the second deferred PR.
Once the prod deploy completes, verify that every PR on the previous StagingDeployCash gets the "deployed to prod" comment.

roryabraham · 2021-05-27T20:53:39Z

HOLD for live testing

roryabraham · 2021-05-27T21:27:47Z

@Dal-Papa All the index.js files here are compiled, so when you review this it's best just to hide them, as they'll be look funky and it will appear to be a lot of duplicate code

Dal-Papa

Asking this question now even though I'm not done with the review to avoid losing too much time.

.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js

Dal-Papa · 2021-05-27T22:51:01Z

@Dal-Papa All the index.js files here are compiled, so when you review this it's best just to hide them, as they'll be look funky and it will appear to be a lot of duplicate code

Just to make sure, when you say compiled, that means that there are multiple source files that end up being the various index.js files right ? I see a lot of duplicate code but it's because of that, right ?

roryabraham · 2021-05-27T22:56:37Z

Just to make sure, when you say compiled, that means that there are multiple source files that end up being the various index.js files right

Yeah, that's correct. A more accurate word than "compiled" would be "bundled with dependencies". Basically any dependencies are bundled into a single Node.js script that has everything it needs to run. There's some more information here

Dal-Papa

It looks good but there's a problem with that validate action ?

roryabraham · 2021-05-28T00:28:52Z

Ah yes, forgot to rebuild the GHA's 🤦

Dal-Papa

✅

roryabraham · 2021-05-28T17:12:48Z

Okay, making a note here that during our testing of CP's over here we discovered another race condition:

The StagingDeployCash is locked, and starts making a new PATCH version./
Another PR is merged to main. Since the StagingDeployCash is locked, it does not create a BUILD version or start a staging deploy. It gets the "deploy deferred" comment.
The PATCH version finishes and is merged to main.
staging is updated from main 💥 – this includes the last-minute PR that was merged while the StagingDeployCash was locked.
So the last-minute PR is deployed, but isn't included in the StagingDeployCash for QA.

Fortunately, the output of this command:

git log --format="%s" 1.0.56-4...1.0.57-0

Includes that last-minute PR. So after the changes from this PR, it would be included in the StagingDeployCash, and not skipped for QA 👍

The last-minute PR should also get the "deployed to staging" comment, so that's good. The only thing that's sort of broken here is that this comment is a bit misleading:

But that's not a severe enough problem to worry about for such a rare edge-case.

roryabraham · 2021-05-28T17:15:22Z

Updated the testing steps of this PR to verify that it fixes this edge case 👍

Dal-Papa · 2021-06-14T08:12:56Z

@AndrewGable : Do you still want to review this or should we get it merged ?

AndrewGable · 2021-06-14T14:50:41Z

Looks good, @roryabraham feel free to self merge when ready to test.

roryabraham · 2021-06-14T16:23:02Z

Thanks, I have to wait to test this until after the next production deploy.

roryabraham · 2021-06-17T16:51:52Z

Going to test this now

OSBotify · 2021-06-17T17:37:56Z

🚀 Deployed to staging in version: 1.0.70-1🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2021-06-17T17:38:53Z

This did not work. First follow-up here just adds better logs to help me diagnose the failure

roryabraham · 2021-06-17T18:15:04Z

Think I found the problem and have a solution here

OSBotify · 2021-06-25T18:31:15Z

🚀 Deployed to production in version: 1.0.73-3🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham added 6 commits May 26, 2021 16:51

Make getPullRequestsMergedBetween synchronous

2dbdd85

Update createOrUpdateStagingDeployAction

7efb9ed

Remove unused functions from GithubUtils

55caf95

Remove unused inputs from action metadata file, rebuild GH actions

9f62575

Add new automated tests

089449c

Rebuild GH actions

8032b7c

roryabraham self-assigned this May 27, 2021

roryabraham changed the title ~~Use full Git history to populate StagingDeployCash~~ [No QA] Use full Git history to populate StagingDeployCash May 27, 2021

roryabraham changed the title ~~[No QA] Use full Git history to populate StagingDeployCash~~ [HOLD] [No QA] Use full Git history to populate StagingDeployCash May 27, 2021

Fix JS style

578aa22

roryabraham marked this pull request as ready for review May 27, 2021 21:25

roryabraham requested a review from a team as a code owner May 27, 2021 21:25

roryabraham requested a review from AndrewGable May 27, 2021 21:26

MelvinBot requested review from Dal-Papa and removed request for a team May 27, 2021 21:26

Dal-Papa reviewed May 27, 2021

View reviewed changes

.github/actions/createOrUpdateStagingDeploy/createOrUpdateStagingDeploy.js Outdated Show resolved Hide resolved

Don't mix template strings and multiple args in console logs

9541798

Dal-Papa previously approved these changes May 28, 2021

View reviewed changes

rebuild GH actions

f6f9676

roryabraham dismissed Dal-Papa’s stale review via f6f9676 May 28, 2021 00:28

Dal-Papa approved these changes May 28, 2021

View reviewed changes

roryabraham mentioned this pull request May 28, 2021

[No QA] Build Cherry-pick flow for Expensify.cash #2978

Merged

roryabraham changed the title ~~[HOLD] [No QA] Use full Git history to populate StagingDeployCash~~ [No QA] Use full Git history to populate StagingDeployCash Jun 2, 2021

roryabraham mentioned this pull request Jun 10, 2021

Deploy Checklist: Expensify.cash 2021-06-08 #3450

Closed

49 tasks

AndrewGable approved these changes Jun 14, 2021

View reviewed changes

roryabraham merged commit 3fcf04f into main Jun 17, 2021

roryabraham deleted the Rory-FullDeployChecklist branch June 17, 2021 16:51

roryabraham mentioned this pull request Jun 17, 2021

[No QA] Add better logging for StagingDeployCash #3642

Merged

roryabraham mentioned this pull request Jun 17, 2021

[No QA] Fetch StagingDeployCash issues in all states #3647

Merged

roryabraham mentioned this pull request Jun 17, 2021

[No QA] Fix StagingDeployCash bug #3650

Merged

OSBotify mentioned this pull request Jun 17, 2021

Deploy Checklist: Expensify.cash 2021-06-15 #3593

Closed

This was referenced Jun 17, 2021

[No QA] Fix StagingDeployCash isAutomatedPullRequest check #3656

Merged

[No QA] Checkout repo before attempting to update StagingDeployCash #3676

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[No QA] Use full Git history to populate StagingDeployCash #3173

[No QA] Use full Git history to populate StagingDeployCash #3173

roryabraham commented May 27, 2021 •

edited

Loading

roryabraham commented May 27, 2021

roryabraham commented May 27, 2021

Dal-Papa left a comment

Dal-Papa commented May 27, 2021

roryabraham commented May 27, 2021

Dal-Papa left a comment

roryabraham commented May 28, 2021

Dal-Papa left a comment

roryabraham commented May 28, 2021

roryabraham commented May 28, 2021

Dal-Papa commented Jun 14, 2021

AndrewGable commented Jun 14, 2021

roryabraham commented Jun 14, 2021

roryabraham commented Jun 17, 2021

OSBotify commented Jun 17, 2021

roryabraham commented Jun 17, 2021

roryabraham commented Jun 17, 2021

OSBotify commented Jun 25, 2021

[No QA] Use full Git history to populate StagingDeployCash #3173

[No QA] Use full Git history to populate StagingDeployCash #3173

Conversation

roryabraham commented May 27, 2021 • edited Loading

Details

Fixed Issues

Tests

roryabraham commented May 27, 2021

roryabraham commented May 27, 2021

Dal-Papa left a comment

Choose a reason for hiding this comment

Dal-Papa commented May 27, 2021

roryabraham commented May 27, 2021

Dal-Papa left a comment

Choose a reason for hiding this comment

roryabraham commented May 28, 2021

Dal-Papa left a comment

Choose a reason for hiding this comment

roryabraham commented May 28, 2021

roryabraham commented May 28, 2021

Dal-Papa commented Jun 14, 2021

AndrewGable commented Jun 14, 2021

roryabraham commented Jun 14, 2021

roryabraham commented Jun 17, 2021

OSBotify commented Jun 17, 2021

roryabraham commented Jun 17, 2021

roryabraham commented Jun 17, 2021

OSBotify commented Jun 25, 2021

roryabraham commented May 27, 2021 •

edited

Loading